feat: centralize deduped public API error alerts (CM-1349) - #4424
Conversation
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Centralizes Redis-deduplicated Slack alerts for public API conflicts and server errors.
Changes:
- Adds centralized
alertOncehandling for 409 and 5xx responses. - Routes alerts to
CDP_PUBLIC_API_ALERTS, excluding Akrites. - Removes entity-specific alert helpers and updates conflict handling.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
services/libs/slack/src/types.ts |
Renames the public API Slack channel. |
services/libs/slack/src/channels.ts |
Maps the renamed channel to its webhook. |
backend/src/api/public/v1/members/resolveMember.ts |
Throws ConflictError directly. |
backend/src/api/public/v1/members/identities/verifyMemberIdentity.ts |
Uses generic conflict handling and removes direct alerts. |
backend/src/api/public/v1/members/identities/createMemberIdentity.ts |
Uses generic database conflict handling. |
backend/src/api/public/v1/members/createMember.ts |
Simplifies conflict mapping. |
backend/src/api/public/middlewares/errorHandler.ts |
Invokes centralized alerting. |
backend/src/api/public/alerts/notifyOnce.ts |
Removes the previous dedupe helper. |
backend/src/api/public/alerts/memberResolveConflict.ts |
Removes member-resolution alert handling. |
backend/src/api/public/alerts/identityConflict.ts |
Removes identity-specific alert handling. |
backend/src/api/public/alerts/alertOnce.ts |
Implements centralized Redis-deduplicated alerts. |
Suppressed comments (3)
backend/src/api/public/v1/members/identities/createMemberIdentity.ts:108
- The centralized alert context drops
memberId, which the removed helper used in both the dedupe key and Slack context. Conflicts for the same identity against different target members will now collapse into one alert, and responders cannot tell which member this request targeted. PassmemberIdthroughrethrowDbConflict.
rethrowDbConflict(error, {
platform: data.platform,
value: data.value,
type: data.type,
})
backend/src/api/public/v1/members/identities/verifyMemberIdentity.ts:100
- This also drops
memberIdfrom the conflict context that feeds the new dedupe key. Attempts to verify the same identity for different members will be treated as duplicates and the resulting alert no longer identifies the target member. PreservememberIdin the mapped conflict.
rethrowDbConflict(error, {
platform: identity.platform,
value: identity.value,
type: identity.type,
})
backend/src/api/public/v1/members/identities/verifyMemberIdentity.ts:173
- Because this workflow-start error is swallowed and the API still returns success, it cannot reach the new error-handler alert path. Deleting the direct Slack call removes the only active alert for an unmerge workflow that never starts; restore explicit alerting or route caught operational failures through a separate centralized mechanism.
} catch (error) {
req.log.warn({ error }, 'Failed to start unmerge workflow after identity unmerge')
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
backend/src/api/public/v1/members/createMember.ts:68
insertMemberIdentitiesnormalizes the stored value, but this context now uses the raw request value. BecausealertOncehashes the context, repeated conflicts forFoo@Example.comandfoo@example.comproduce different keys and bypass the intended deduplication. Preserve the normalization that was removed here.
value: identity.value,
backend/src/api/public/v1/members/identities/verifyMemberIdentity.ts:154
- This error is caught and the request continues successfully, so the centralized error handler can never alert on it. Removing the explicit notification therefore makes post-unmerge audit failures visible only in logs; retain an operational alert for this swallowed failure.
} catch (error) {
req.log.warn({ error }, 'Audit log capture failed after identity unmerge')
backend/src/api/public/v1/members/identities/verifyMemberIdentity.ts:173
- This workflow-start failure is swallowed after logging, so it never reaches
errorHandleroralertOnce. Removing the existing Slack notification silently eliminates the alert for an unmerge whose follow-up workflow did not start; keep a direct operational alert in this catch path.
} catch (error) {
req.log.warn({ error }, 'Failed to start unmerge workflow after identity unmerge')
backend/src/api/public/middlewares/errorHandler.ts:33
- Known 500s such as
InternalError('Failed to update member identity')take thisHttpErrorbranch, which neither logs them nor includes their stack in the alert. Pass the stack for 5xx errors so the new centralized alert remains actionable, matching the unhandled-error branch below.
name: error.name,
context: error instanceof ConflictError ? error.context : undefined,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (5)
backend/src/api/public/v1/members/createMember.ts:68
- The DAL normalizes identity values before insertion, but this context now keeps the raw value. Requests for the same email with different case or surrounding whitespace therefore hit the same unique conflict while producing different dedupe hashes and repeated alerts. Restore the normalization used here previously.
value: identity.value,
backend/src/api/public/v1/members/identities/verifyMemberIdentity.ts:154
- This failure is caught and the request still succeeds, so the centralized error handler never sees it. Removing the existing Slack call silently drops the operational alert for failed audit capture; retain a channel-specific notification or explicitly forward this caught failure to an alerting path.
req.log.warn({ error }, 'Audit log capture failed after identity unmerge')
backend/src/api/public/v1/members/identities/verifyMemberIdentity.ts:173
- This exception is swallowed after logging, so it cannot reach
errorHandleror the newalertOncepath. Deleting the Slack notification removes the only immediate alert that the identity was unmerged in the database but its follow-up workflow did not start; keep a dedicated operational alert for this partial-failure state.
req.log.warn({ error }, 'Failed to start unmerge workflow after identity unmerge')
backend/src/api/public/alerts/alertOnce.ts:118
- The delimiter-based format can map different contexts to the same hash input. For example,
{ a: 'x|b=y' }and{ a: 'x', b: 'y' }both serialize toa=x|b=y, allowing one distinct alert to suppress another. Use a canonical, key-sorted JSON serialization that preserves value boundaries.
return `${key}=${String(value)}`
backend/src/api/public/alerts/alertOnce.ts:102
- When an error exits the nested
/v1/...routers, Express restoresreq.baseUrlbefore this parent error middleware runs, whilereq.route.pathremains only the leaf path. Routes such as member creation, organization creation, and stewardship creation therefore all resolve to/; matching 500 messages can suppress each other for an hour. Build the key from the full original path instead.
return `${req.baseUrl}${req.route.path}`
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (6)
backend/src/api/public/v1/members/identities/createMemberIdentity.ts:108
memberIdwas part of the removed identity-conflict alert key and payload. Omitting it here makes attempts to attach the same identity to different destination members collapse into one alert and removes the affected member from the diagnostic context. Include it in theConflictErrorcontext passed to the centralized handler.
rethrowDbConflict(error, {
platform: data.platform,
value: data.value,
type: data.type,
})
backend/src/api/public/v1/members/identities/verifyMemberIdentity.ts:100
- This drops
memberIdfrom the alert context and dedupe key that the removed helper previously used. As a result, verification conflicts for the same identity on different members are treated as duplicates, and responders cannot see which destination member triggered the conflict.
rethrowDbConflict(error, {
platform: identity.platform,
value: identity.value,
type: identity.type,
})
backend/src/api/public/v1/members/identities/verifyMemberIdentity.ts:154
- This exception is swallowed, so it never reaches
errorHandler; removing the direct notification means an audit-log failure after a successful unmerge now produces no Slack alert at all. Route this caught failure throughalertOnce(with a distinct code and member context) before continuing, or retain the direct notification.
} catch (error) {
req.log.warn({ error }, 'Audit log capture failed after identity unmerge')
backend/src/api/public/v1/members/createMember.ts:68
- The removed code normalized this value before constructing the conflict alert. The database also normalizes identity values, so equivalent requests such as differently cased or whitespace-padded emails now produce different centralized dedupe keys and can send repeated alerts for the same conflict. Preserve normalization in the context.
value: identity.value,
backend/src/api/public/v1/members/identities/verifyMemberIdentity.ts:173
- Because this workflow-start error is caught and the request still succeeds, the centralized error handler cannot alert on it. Removing the existing notification silently eliminates monitoring for failed post-unmerge processing; emit an
alertOncenotification here (with primary/secondary IDs), or preserve the direct alert.
} catch (error) {
req.log.warn({ error }, 'Failed to start unmerge workflow after identity unmerge')
.github/scripts/notify-api-e2e-failure.sh:15
failed_steponly checks four steps, but the notification runs for failures inInstall OCIandUpdate PATHtoo. Those failures will be reported asunknown, defeating the new diagnostic field. Give those workflow steps IDs, pass their outcomes, and include them in this list.
failed_step() {
local pairs=(
"${OUTCOME_RESOLVE_TAG}:Resolve deploy image tag"
"${OUTCOME_DEPLOY}:Deploy api-e2e"
"${OUTCOME_HEALTH}:Wait for api-e2e service to be ready"
"${OUTCOME_E2E}:Run e2e tests"
)
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
…nction Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/api-e2e-tests.yml:99
- If
actions/checkoutfails, this failure handler still runs but the checked-in script does not exist, so the Slack alert is lost. The previous inline handler could report checkout failures. Keep a minimal inline fallback (or use an external action/script available without checkout) so every workflow failure can still notify.
run: bash .github/scripts/notify-api-e2e-failure.sh
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
backend/src/api/public/alerts/alertOnce.ts:93
- For a non-500 server error such as 503, the alert title is still labeled
500 Erroreven though the function accepts and alerts on every 5xx status. This makes incident notifications report the wrong status; use the actualstatusvalue.
status >= 500 ? `500 Error: ${name || message}` : `${status} Conflict: ${message}`,
Summary
Moves public API Slack alerting into a single Redis-deduped
alertOncepath from the error handler, so 409 and 5xx alerts go toCDP_PUBLIC_API_ALERTSwithout spamming repeats. Entity-specific conflict alert helpers are removed; Akrites routes are skipped (separate channel).Changes
alertOncewith hashed dedupe key (status/ method / route / code / message / context)errorHandlerto callalertOnceforHttpErrorand unhandled 500sidentityConflict,memberResolveConflict, andnotifyOncealert modulesrethrowDbConflict/ bareConflictErrorCDP_LFX_SELF_SERVE_ALERTS→CDP_PUBLIC_API_ALERTS